Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix dtype issues #493

Merged
merged 14 commits into from
Sep 7, 2021
Merged

Fix dtype issues #493

merged 14 commits into from
Sep 7, 2021

Conversation

SkafteNicki
Copy link
Member

@SkafteNicki SkafteNicki commented Sep 2, 2021

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?

What does this PR do?

Fixes #492
Fixes #484

Reverts some of the changes in #462.

  • Removes the dtype property. Metrics state can have different dtypes (like any other nn.Module) so the property probably does not make sense for all metrics
  • Fixes bug with mixed precision training: half, float, double will now have no effect on the dtype of metric states to prevent that metrics gets auto-cast during mix-precision. Instead if the user really wants to change the dtype of all metric states they can now do it using the set_dtype method.

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

@SkafteNicki SkafteNicki added the bug / fix Something isn't working label Sep 2, 2021
@codecov
Copy link

codecov bot commented Sep 2, 2021

Codecov Report

Merging #493 (3921a24) into master (f96c717) will decrease coverage by 23%.
The diff coverage is 10%.

@@           Coverage Diff           @@
##           master   #493     +/-   ##
=======================================
- Coverage      95%    72%    -23%     
=======================================
  Files         132    132             
  Lines        4681   4676      -5     
=======================================
- Hits         4462   3365   -1097     
- Misses        219   1311   +1092     

@Borda Borda marked this pull request as draft September 2, 2021 13:52
@Borda Borda changed the title [WIP] Fix dtype issues Fix dtype issues Sep 2, 2021
@SkafteNicki SkafteNicki marked this pull request as ready for review September 2, 2021 16:36
@Borda
Copy link
Member

Borda commented Sep 2, 2021

@ethanwharris mind have look? ^^

Copy link
Contributor

@tchaton tchaton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM !

Copy link
Member

@ethanwharris ethanwharris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, confirmed that this fixes my issue 😃 thanks @SkafteNicki !

@Borda Borda enabled auto-merge (squash) September 2, 2021 23:20
@Borda Borda added this to the v0.5 milestone Sep 2, 2021
@mergify mergify bot added ready and removed ready labels Sep 3, 2021
@Borda Borda requested a review from ananthsub September 3, 2021 12:29
@Borda
Copy link
Member

Borda commented Sep 6, 2021

@SkafteNicki seems some tests are running for too long with latest versions

@mergify mergify bot added the has conflicts label Sep 6, 2021
requirements/test.txt Outdated Show resolved Hide resolved
tests/helpers/testers.py Outdated Show resolved Hide resolved
@mergify mergify bot removed the has conflicts label Sep 7, 2021
tests/helpers/testers.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug / fix Something isn't working
Projects
None yet
5 participants